Hold on review for a bit, re-examining. Definitely needs to combine probabilities of each reroll state, and refactoring it to fit more in line with other modifiers

The math is kind of confusing at times, but I was so happy to see your Pull Request because I didn't want to write this myself. I pulled your code into my local machine and have been playing with it, but I think the Lucky stuff is wrong. Specifically, I was trying to get the node Lightning Rod to work for me with 30% Lucky on Lightning hits, and that led me to looking at how some of the math is done around Lucky.

@Dimencia can you review this code below and see if it could be integrated or confirmed as correct for your Pull Request? The changes are primarily around the calculation for CritChanceLucky and I added a link in the comments to how that should be calculated.

Thanks again for doing all the work!

elseif activeSkill.skillModList:Flag(nil, "InevitableCriticalHits") then
	-- Calculate average number of rerolls for a non-crit
	-- Use pre-effective so we don't consider accuracy, which already scales DPS
	local critChance = output.PreEffectiveCritChance
	local critChancePct = critChance / 100						

	-- Consider lucky crits because they were only applied post-effective
	-- (not that they exist in POE2 for now, but just in case)
	if skillModList:Flag(cfg, "CritChanceLucky") then
		--https://www.poewiki.net/wiki/Luck
		critChance = (2*critChancePct - critChancePct^2) * 100
	else		
		--Apply lucky chance to crit rolls when we're calculating the scalar for InevitableCriticalHits
		damageTypeLuckyChance = m_min(skillModList:Sum("BASE", skillCfg, damageType.."LuckyHitsChance", "LuckyHitsChance"), 100) / 100							
		if(damageTypeLuckyChance > 0) then							
			--at 100% luck, our new crit chance is: critChance = 2*critChance - critChance^2
			--but we are probably below 100% luck, so let's scale down the new critChance 
				maxLuckCritChancePct = 2*critChancePct - critChancePct^2

			--Get our new crit chance by adding the partial bonus from Max Luck to our base crit
			critChance = ((1-damageTypeLuckyChance)*critChancePct + damageTypeLuckyChance*maxLuckCritChancePct) * 100								
		end
	end

	local avgNumRerolls = 100 / critChance - 1
	local critBonusMultiplier = 1 - m_min(1, .3 * avgNumRerolls)

	-- Crit multiplier includes the base 100%, +some% bonus
	--   but this penalty only applies to the some% bonus
	local bonusMult = output.CritMultiplier - 1
	local modifiedBonus = bonusMult * critBonusMultiplier
	local newCritMult = 1 + modifiedBonus
	allMult = allMult * newCritMult

	if breakdown then
		t_insert(breakdown[damageType], "")
		t_insert(breakdown[damageType], "Inevitable Criticals: ")
		t_insert(breakdown[damageType], s_format("  Base Crit Bonus: +%.2f%%", bonusMult * 100))
		t_insert(breakdown[damageType], s_format("  Avg Num Rerolls: %.2f", avgNumRerolls))
		t_insert(breakdown[damageType], s_format("  Avg Crit Bonus: +%.2f%%", modifiedBonus * 100))
		t_insert(breakdown[damageType], s_format("x %.2f ^8(Inevitable Crit Multiplier)", newCritMult))
		t_insert(breakdown[damageType], "")
	end
end

I should note regarding my code I just posted that I don't test for > 100% lucky and don't test bounds with m_max. Lua is still super new to me.

Review can commence - I think the logic is fixed and in a better spot, renamed things to "ForcedOutcome" everywhere to avoid confusion with the inevitable critical support gem, and added some tests

@jaredschnelle The node you're looking at is '30% chance for Lightning Damage with Hits to be Lucky' - the damage specifically is lucky, not the hit itself (such as its accuracy or crit chance), at least as far as I can tell and judging by existing code

The lucky crit formula I'm using actually works out to be the same thing as your critChance = (2*critChancePct - critChancePct^2) * 100. I ended up refactoring it to make it a bit easier to look at, but it should work out to the same crit chance

If you were seeing problems, it's probably because I wasn't really considering probabilities right, fixed in the latest PRs

Yes, "damage lucky" is distinct from "critical hit chance is lucky". Afaik there's currently no "x% chance for critical hit chance to be lucky" in either game. It's binary on, or off.

However, please note that both "lucky" and "unlucky", as well as "bifurcated" criticals count as "re-rolls" and would therefore cause the penalty from "Inevitable Critical" to be applied.

Thank you both for the clarification. This game does have some crazy math inside of it. I didn't realize that the "Hit" happens before the "Damage" roll. Also, the fact that Rerolling from things like Lucky or Bifurcation diminish the crit damage applied really does change the way I was looking at things.

It'd be interesting to get Bifurcation working with Inevitable Outcomes seeing as how the 2nd roll on Bifurcate will always have a lower Crit Damage Bonus. I realize this isn't strictly related to this PR, so I'll end it here. Thanks again for the insight.

I pushed some changes recently that should address that, and am still working on more... kinda went a bit overboard refactoring things and need to put most of it back

How it interacts with bifurcate/lucky is definitely weird and worth some discussion

Personally I think that the wording of Bifurcation implies that it occurs once per hit, not once per roll: Hits with this weapon roll against their Critical Hit chance twice when determining if they are a Critical Hit. I expect that the bifurcated reroll happens only once, on the first roll and not any subsequent ones

But on the other hand, I think the wording of lucky crits implies that it is in fact per roll: Your critical hit chance is lucky - it's your crit chance itself that's lucky, not your hits. This seems to imply that every time forced outcome rerolls against crit chance, it should be lucky

So my current approach is to use the full crit chance, which already scaled to consider the bifurcated reroll, for only the first hit's probability (and separately, just globally apply 30% less crit damage bonus if bifurcation is on, because it always rolls). As for lucky hits, I assume they're rolling twice every time forced outcome rerolls - meaning we shouldn't discard its effect on crit chance after the first hit (like with bifurcatino), but also that you end up with a 60% penalty per reroll (on top of a global 30% less for having lucky at all)

I'm a little on the fence because that seems very harsh, but either way, I don't think you want to pair either one with forced outcome

Here's a snippet of the relevant logic that I'm still considering

-- The wording for Bifurcated Crits implies it occurs per hit, not per crit chance roll, so we'll only use it for the first hit
local bifurcateCritChance = output.CritChance / 100
local bifurcateNonCritChance = 1 - bifurcateCritChance

-- "Your critical hit chance is lucky" implies that it occurs every time we roll, not per hit (which means a 60% penalty per roll)
local critChance = output.PreBifurcateCritChance / 100
local nonCritChance = 1 - critChance
local multStep = skillModList:Flag(cfg, "CritChanceLucky") and 0.6 or 0.3

local critBonusMultiplier =
	1 * bifurcateCritChance +
	m_max(0, 1 - multStep) * bifurcateNonCritChance * critChance +
	m_max(0, 1 - multStep * 2) * bifurcateNonCritChance * nonCritChance * critChance +
	m_max(0, 1 - multStep * 3) * bifurcateNonCritChance * nonCritChance * nonCritChance * critChance